-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BBT-123]: Add functionality to allow editors to manage multiple style variations #51
Conversation
inc/class-rest-api.php
Outdated
foreach ( $posts as $post ) { | ||
$post_status = 'draft'; | ||
if ( $post->ID === $global_styles_id ) { | ||
$post_status = 'publish'; | ||
} | ||
wp_update_post( | ||
array( | ||
'ID' => $post->ID, | ||
'post_status' => $post_status, | ||
) | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just flagging this as potentially slow, it will perform wp_update_post
on all posts regardless of whether they are already draft
and there could be many.
We expect (but can't assume) that there's only going to be 1 published post at a time we need to revert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense - I've added an extra check at the beginning of the loop so any posts that aren't already published and aren't going to be published can be ignored.
if ( 'publish' !== $post->post_status && $post->ID !== $global_styles_id ) {
continue;
}
inc/class-rest-api.php
Outdated
$posts = get_posts( | ||
array( | ||
'post_type' => 'wp_global_styles', | ||
'post_status' => array( 'publish', 'draft' ), | ||
'orderby' => 'date', | ||
'order' => 'desc', | ||
'numberposts' => -1, | ||
'ignore_sticky_posts' => true, | ||
'no_found_rows' => true, | ||
'update_post_meta_cache' => false, | ||
'update_post_term_cache' => false, | ||
'tax_query' => array( // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_tax_query -- This could be a slow query, but it's necessary. | ||
array( | ||
'taxonomy' => 'wp_theme', | ||
'field' => 'name', | ||
'terms' => $theme, | ||
), | ||
), | ||
) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a potentially slow query we may want to avoid running it until necessary.
During POST
requests we already have the post ID, so can check it exists and run appropriate checks first. It may also be better to split this function up to handle POST and GET logic separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a couple of changes related to your comment:
- I've split this function up so each request method calls a separate method in the class.
- I've moved the
get_posts
segment into its own utility function, so it can be called when needed by each method, and will likely come in handy in the future. - I'm now checking if the
$global_styles_id
is a valid post before grabbing all the posts.
also created new util function for getting the theme styles posts
inc/class-rest-api.php
Outdated
if ( ! $global_styles_id || ! get_post_status( $global_styles_id ) ) { | ||
return new WP_Error( 'invalid_global_styles_id', __( 'Invalid global styles ID', 'themer' ) ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may need to do some additional checks here. Imagine you are accessing the REST API directly and can therefore pass any value to the global_styles_id
. We should:
- Check the post type is correct
- Check the post is related to the active theme via taxonomy term
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've refactored this further now to include the checks you've mentioned. I've moved this logic into a validation_callback
in the register_rest_route
call, so this will all be checked before we even run the main endpoint logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great, thanks @Joe-Rouse
Description
Completes BBT-123 - This PR adds the ability for an editor to manage multiple styles at once, and to select which one of those styles is 'active'. The UI is very basic here and will change in the future, but this PR just focuses on adding the basic functionality.
I've reused the endpoint from BBT-122, but changed it up slightly so it handles both GET & POST requests - a GET request allows us to retrieve all the style variations for the current theme, and a POST request sets the active style variation for the current theme.
Rather than grabbing the
globalStylesId
using the core__experimentalGetCurrentGlobalStylesId()
function, I'm now making a request to our endpoint and finding the post that is published as there should only ever be 1 for the current theme.Change Log
<select>
control in the top bar which allows us to switch between variationsActivate
button which sets the variation that's currently being edited as the active variationSteps to test
wp_global_styles
posts linked to the current theme #50Screenshots/Videos
Screen.Recording.2024-01-29.at.13.37.29.mov
Checklist: